Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove label from dataview checkbox #67868

Open
wants to merge 14 commits into
base: trunk
Choose a base branch
from
Open

Remove label from dataview checkbox #67868

wants to merge 14 commits into from
+6 −15

Conversation

karthick-murugan
Copy link
Contributor

What?

Fixes #59178

This PR updates the labels associated with checkboxes in data views to remove the dynamically generated state values (Select item: and Deselect item:). The labels now display only the relevant item title, simplifying and improving accessibility.

Why?

The current implementation of checkbox labels includes dynamic state indicators (Select item: and Deselect item:) which
overcomplicate the labels and do not adhere to the semantic purpose of labels as descriptors of the control.

How?

Removed the dynamic prefixes (Select item: and Deselect item:) from the checkbox labels in the code. Updated the label to display only the item's title, derived from the provided titleField property.

Testing Instructions

  • Navigate to the Site Editor > Pages > Manage all pages.
  • Hover over a table row to make the checkbox visible.
  • Verify that the label for each checkbox contains only the item's title.
  • Select and deselect a checkbox.
  • Confirm that the label text remains unchanged and only displays the item title.

Screenshots or screencast

REC-20241212152215.mp4

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 12, 2024 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: karthick-murugan <[email protected]>
Co-authored-by: afercia <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: alexstine <[email protected]>
Co-authored-by: ntsekouras <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@karthick-murugan
Copy link
Contributor Author

@t-hamano , @afercia - Please have a look at this PR when you get some time. Thanks in advance.

@afercia
Copy link
Contributor

afercia commented Dec 18, 2024

I'd totally support not using dynamic labels. Labels determine the accessible name of a control, which should rarely change dynamically. For checkboxes, the checked state is implied and natively communicated to assistive technologies.

@afercia
Copy link
Contributor

afercia commented Dec 18, 2024

A pre-existing issue is that selectionLabel may be not set. In that case, the checkbox would be unlabelled which is a no-go for accessibility. Unfortunately the base component like CheckboxControl do not enforce the label prop so that I wonder if we should provide a fallback label here.

afercia
afercia reviewed Dec 18, 2024
View reviewed changes
Copy link
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't aria-label={ selectionLabel } be just the label prop?

@karthick-murugan
Copy link
Contributor Author

Shouldn't aria-label={ selectionLabel } be just the label prop?

Replacing aria-label with the label prop results in the label being visually displayed next to the checkbox (which may not be desirable in this context)

A pre-existing issue is that selectionLabel may be not set. In that case, the checkbox would be unlabelled which is a no-go for accessibility. Unfortunately the base component like CheckboxControl do not enforce the label prop so that I wonder if we should provide a fallback label here.

You’re absolutely right, an unlabelled checkbox would be a critical accessibility issue. I will ensure that if the selectionLabel is not set or is empty, a fallback label such as "Select item" or the item's title is used as a default.

@karthick-murugan
Copy link
Contributor Author

@afercia, @t-hamano - Updated the code such that, default fallback ('Select item') is provided to ensure that every checkbox is accessible even if titleField is not defined.

t-hamano
t-hamano approved these changes Dec 21, 2024
View reviewed changes
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes sense to me. Screen readers appear to work properly both with and without titles:

54d09f32641d1b647adf7d87639487f7.mp4

@afercia Any additional feedback would be appreciated.

@karthick-murugan
Copy link
Contributor Author

@afercia - Please let us know if you have any additional comments, or else we can move forward. Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afercia afercia afercia left review comments

@t-hamano t-hamano t-hamano approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Data views: Improve the checkboxes labels
3 participants
@karthick-murugan @afercia @t-hamano